(The existing remove logic only stored a logical delete flag, null, in the cleared entry and therefore was leaking memory.)

@luc-blaeser I do not follow this claim.

The existing logic, as with the new logic proposed here, uses association lists. Those lists then must do the delete physically for the Trie or TrieMap to have this property.

Looking at the existing replace logic (which subsumes both insert and remove and implements both with a more general type), I see that it does in fact "shrink" the list when it finds the target element of the remove (again, phrased as a replace):

        case (?((hd_k, hd_v), tl)) {
          if (equal(key, hd_k)) {
            // if value is null, remove the key; otherwise, replace key's old value
            // return old value
            switch value {
              case (null) { (tl, ?hd_v) };
              case (?value) { (?((hd_k, value), tl), ?hd_v) }
            }
          } else {
          ...
          }

So we see that in the subcase for null, the copied list really is shrinking.

I realize that this PR is doing some orthogonal improvements around this operation, but I remain very confused by the claim that the existing logic does not actually release memory (after GC over a TrieMap that uses this logic, it should, right)?

(The existing remove logic only stored a logical delete flag, null, in the cleared entry and therefore was leaking memory.)

@luc-blaeser I do not follow this claim.

The existing logic, as with the new logic proposed here, uses association lists. Those lists then must do the delete physically for the Trie or TrieMap to have this property.

Looking at the existing replace logic (which subsumes both insert and remove and implements both with a more general type), I see that it does in fact "shrink" the list when it finds the target element of the remove (again, phrased as a replace):

        case (?((hd_k, hd_v), tl)) {
          if (equal(key, hd_k)) {
            // if value is null, remove the key; otherwise, replace key's old value
            // return old value
            switch value {
              case (null) { (tl, ?hd_v) };
              case (?value) { (?((hd_k, value), tl), ?hd_v) }
            }
          } else {
          ...
          }

So we see that in the subcase for null, the copied list really is shrinking.

I realize that this PR is doing some orthogonal improvements around this operation, but I remain very confused by the claim that the existing logic does not actually release memory (after GC over a TrieMap that uses this logic, it should, right)?

Thanks Matthew for pointing this out. You are right, I found a mistake in my benchmark case (deleting less than allocating). I am very sorry for this confusion.

So, after re-measuring (hopefully correct this time), the benchmark shows that a majority of the memory can be reclaimed after deleting the entries in the TrieMap. Only the (empty) leaves in the trie remain after delete. This PR only adds a small optimization insofar that it also combines and frees some of the leaf nodes.

Measurement results, creating 1_000_000 (Nat, Nat) entries in a TrieMap, then removing all of them (several rounds):

Heap size

The question is on whether we should consider this PR or close it for the moment.

@luc-blaeser Okay, I am relieved. No worries!

Before you mentioned finding an actual measurement-related bug, I started wondering if the Trie is just so wasteful (rebuilding those little lists in a pure way) that it seemed like there was a leak, as you alluded to today in the meeting.

FWIW, I apologize for the Trie being cryptic in its implementation. When I started rereading that code snippet this morning I started doubting if I even understood it, then I did again, eventually. :)

This PR only adds a small optimization insofar that it also combines and frees some of the leaf nodes.

In addition to the space trimming for delete, the PR also adds more unit tests that are randomized.

Both of those improvements seem valuable to me!

BTW, I didn't consider these cases for compressing subtrees when I was "fixing" the earlier, broken way that filter had done it.

Now, filter uses this logic to compress its output tree:

          if (isEmpty(fl) and isEmpty(fr)) {
            #empty
          } else {
            branch(fl, fr)
          }

But it's missing some compression opportunities when one subtree is a leaf and the other is null.

Your logic in the PR could expand to improve filter too, it seems?

The space savings (and more tests) still look worthwhile to me, so I'd suggest continuing with the PR or merging in its current state (perhaps with TODO to improve filter and mapFilter (if we have that)..

Is it possible to get to the situation where removing all the elements from a tree gets you back to #empty or is that just too expensive to arrange? I'm just curious, not necessarily suggesting we reach for that goal.

The space savings (and more tests) still look worthwhile to me, so I'd suggest continuing with the PR or merging in its current state (perhaps with TODO to improve filter and mapFilter (if we have that)..

Is it possible to get to the situation where removing all the elements from a tree gets you back to #empty or is that just too expensive to arrange? I'm just curious, not necessarily suggesting we reach for that goal.

I tested it, and the tree is actually empty again after the removal of all entries. I will also look at the filter and mapFilter soon.

Update: The empty node collapsing logic has been applied to filter and mapFilter too.

Nice. Just made some minor suggestions to remove parens and perhaps optimize replace, but all optional.

There's probably similar spurious parens in patterns in the rest of the file, but I can't make GH suggestions on unchanged lines. Probably not worth fixing.

Thanks a lot for all these valuable suggestions. Would you like to push the parentheses optimization?